-
-
Notifications
You must be signed in to change notification settings - Fork 651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to React Native v63. #4420
Conversation
These are the RN commits that touch the RN template app that are in v0.63.4 but are not in v0.62.2. They're ordered from first to last (oldest to newest). I.e., they're the commits in the output of Along with whether, how, and when we propagate the changes to our app. We shouldn't forget to update this, if necessary, to follow any changes made during code review and merge.
|
This comment describes an issue that I ran into and solved with a simple command; the result is in the current revision. On running
I then ran
This second message matches the one reported in facebook/react-native#30015. The suggested solution there is to run the command recommended in the output itself, The command seems to work for us. I observed that the So, I ran and committed the result of |
In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - TODO: Do something about react-native-cameraroll. - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread [2] for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [4]. [1] See zulip#4420 (comment) for more details on this. [2] zulip#4420 (comment) [3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [4] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file. Fixes: zulip#4245
Thanks @chrisbobbe ! I'll look closer at this today.
If your PR there (or some alternative) isn't merged upstream when we're otherwise ready to merge this upgrade, we can always handle this by pointing to your PR commit. But the project does seem to be actively merging PRs, and I see you left a ping on the issue just today, so hopefully that state won't last long. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for all your work on this upgrade, and for the clear table above. A few comments below.
// Should be fixed in RN v0.63 (#4245); see | ||
// https://github.com/zulip/zulip-mobile/issues/4245#issuecomment-695104351. | ||
// $FlowFixMe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the `$FlowFixMe`s start getting flagged as "unused suppression
comments", remove them.
Strangely, `this.textInputRef.current` and friends are
`any(implicit)` after this.
Hmm -- that is concerning. And indeed if I go and change these method calls to nonsense names, I get no error.
I think this may ultimately all be down to bugs in Flow's special support for React. Both React$ElementRef
, and React$AbstractComponent
aka React.AbstractComponent
, which is part of the TextInput
type, are implemented by special logic within Flow. I suspect that that logic isn't fully working as intended.
If we're not going to get type-checking on these anyway, I think I'd rather have the fixmes there, so it's obvious looking at the code that it isn't type-checked and you need to be careful about the types by hand.
src/common/InputWithClearButton.js
Outdated
@@ -32,7 +32,7 @@ export default class InputWithClearButton extends PureComponent<Props, State> { | |||
canBeCleared: false, | |||
text: '', | |||
}; | |||
textInputRef = React.createRef<typeof TextInput>(); | |||
textInputRef = React.createRef<React$ElementRef<typeof TextInput>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar change should also happen at Input
in the type of its prop that we pass this to.
(Though maybe not now after all; see previous comment.)
pod 'React', :path => '../node_modules/react-native/' | ||
pod 'React-Core', :path => '../node_modules/react-native/' | ||
pod 'React-CoreModules', :path => '../node_modules/react-native/React/CoreModules' | ||
pod 'React-Core/DevSupport', :path => '../node_modules/react-native/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change here is that this particular pod no longer gets included. The upstream function has this:
unless production
pod 'React-Core/DevSupport', :path => "#{prefix}/"
end
and production
defaults to false
.
I'm not sure what that does. It sounds like something that's good to leave out in release builds, but we might miss in dev builds. But I guess if things still seem to work well in development, then we didn't need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since production
defaults to false
, and we don't take it away from the default (which we would do by passing an argument with our use_react_native!
call in the Podfile), I think this pod is still included in all builds.
unless false
// This code will definitely run
end
unless false
is equivalent to if true
, I think. Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just changed it to unless true
in node_modules and run pod install
(via yarn
), and I see a line with React-Core/DevSupport
removed from Podfile.lock
.
(The /
says to look for DevSupport
as a "subspec" of React-Core
. I think all subspecs of React-Core
would normally have been pulled in by the pod 'React-Core'
line, above this conditional—but this isn't happening here because React-Core
's podspec declares a "default subspec"; doc.)
Probably best to keep pulling in DevSupport
for development, but I agree that it should be fine to exclude it in production builds. I've just tried doing a release build with it excluded, and it built with no errors and is so far running just fine on my iPhone 6s.
I'd be nice if React Native took the approach it took with use_flipper!
in facebook/react-native@c72ecdb (see also our 61bda2a) toward use_react_native!
. The "build configuration" (the thing we use at build time to say whether we're building for production or development) isn't known at pod install
time, which is the time the Podfile is run. So I don't see a way we could pass a correct value for the production
param that use_react_native!
expects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't see a way we could pass a correct value for the
production
param thatuse_react_native!
expects.
Given this, it's possibly just fine to not do anything for now. Including React-Core/DevSupport
in release builds is the status quo anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless false
is equivalent toif true
, I think. Is that right?
Ha, yes, I guess I misread that code 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with all the above. This change is NFC -- we were always getting this pod, and we're still always getting it. And the appropriate thing would be for that function to do something like what use_flipper!
does, and take a list of configurations for it to apply in.
(Or else have some other way of being conditioned on the configuration.)
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - TODO: Do something about react-native-cameraroll. - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread [2] for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [4]. [1] See zulip#4420 (comment) for more details on this. [2] zulip#4420 (comment) [3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [4] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file. Fixes: zulip#4245
Thanks for the review, @gnprice! I've just pushed a new revision that addresses #4420 (comment) and an additional type-checking thing that had slipped through the cracks. I've also responded to the point about Next, I'll go ahead and try pointing to my PR commit for react-native-cameraroll, as you've suggested at #4420 (comment). I'm not 100% sure there isn't some kind of necessary building/packaging step that contributes to what we get from NPM; I'll find out soon enough. 🙂 |
All looks good, thanks! And that sounds like a good plan. |
As Greg suggests [1], point to the commit at the tip of my PR react-native-cameraroll/react-native-cameraroll#264. Once a version of @react-native-community/cameraroll with official RN v0.63 support (including as declared in `peerDependencies`) is released to NPM, we should go back to specifying a version range, making sure that that range doesn't include versions before that update. We expect this will happen relatively soon; the project does seem to be actively merging PRs, and I recently left a ping on the issue. But we'd like to start using RN v0.63 in our project ASAP, so we use this strategy in the meantime. Sometimes packages have a crucial build/packaging step that gets done as part of releasing a version to NPM. Such a step would likely make it impossible to point to a commit on GitHub and have the package still work. But I've verified that it does indeed work when we point to the commit on GitHub, so there's no problem there. [1] zulip#4420 (comment)
In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread [2] for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [4]. We know of one bug that's directly solved by taking this upgrade: issue zulip#4365. I've just tested, and I do indeed start to see avatars on a simulator running iOS 14, where I didn't see them before. [1] See zulip#4420 (comment) for more details on this. [2] zulip#4420 (comment) [3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [4] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file. Fixes: zulip#4245 Fixes: zulip#4365
Great, thanks; I've just added a commit that points to the PR commit on react-native-cameraroll/react-native-cameraroll#264. I suppose it would be slightly neater to have this commit live on a branch owned by I've also unmarked the PR as a draft, and marked the upgrade commit as fixing #4365, after testing and confirming that it does in fact fix it, as expected. |
We should be passing a `React$ElementRef` type as a type argument to `React.createRef`; see zulip#4278 (comment).
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
As Greg suggests [1], point to the commit at the tip of my PR react-native-cameraroll/react-native-cameraroll#264. Once a version of @react-native-community/cameraroll with official RN v0.63 support (including as declared in `peerDependencies`) is released to NPM, we should go back to specifying a version range, making sure that that range doesn't include versions before that update. We expect this will happen relatively soon; the project does seem to be actively merging PRs, and I recently left a ping on the issue. But we'd like to start using RN v0.63 in our project ASAP, so we use this strategy in the meantime. Sometimes packages have a crucial build/packaging step that gets done as part of releasing a version to NPM. Such a step would likely make it impossible to point to a commit on GitHub and have the package still work. But I've verified that it does indeed work when we point to the commit on GitHub, so there's no problem there. [1] zulip#4420 (comment)
In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread [2] for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [4]. We know of one bug that's directly solved by taking this upgrade: issue zulip#4365. I've just tested, and I do indeed start to see avatars on a simulator running iOS 14, where I didn't see them before. [1] See zulip#4420 (comment) for more details on this. [2] zulip#4420 (comment) [3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [4] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file. Fixes: zulip#4245 Fixes: zulip#4365
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
These obviously didn't get fixed in the RN v0.63 upgrade, as we'd thought they would... It turns out that it's a very similar bug [1] to the one we originally thought it was [2], but slightly different. `UserPresence` is an object type with an indexer *and* an additional property. [1] facebook/flow#8276 [2] facebook/flow#8178
Looks great -- merged!
I think that would definitely be better if we found ourselves maintaining a fork of that repo, with ongoing changes. (As we did for remotedev-serialize, starting at 9d0aa95, before we ended up inlining that code and integrating it more closely into the main mobile repo.) But when the intent really is "just this one PR, until it's hopefully merged soon", I think pointing straight to the branch the PR was sent from fits pretty well. If in a few months we notice that it's still there and the PR isn't merged, and there's some other maintenance it needs, we might end up moving to the fork model; but hopefully we'll instead get to just switch back to pointing upstream, and then get to forget about this package's maintenance again for a while. |
Great, thanks! 🙂
Makes sense. I've just opened #4425 so we don't totally forget to switch back to a published version on NPM. |
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
As Greg suggests [1], point to the commit at the tip of my PR react-native-cameraroll/react-native-cameraroll#264. Once a version of @react-native-community/cameraroll with official RN v0.63 support (including as declared in `peerDependencies`) is released to NPM, we should go back to specifying a version range, making sure that that range doesn't include versions before that update. We expect this will happen relatively soon; the project does seem to be actively merging PRs, and I recently left a ping on the issue. But we'd like to start using RN v0.63 in our project ASAP, so we use this strategy in the meantime. Sometimes packages have a crucial build/packaging step that gets done as part of releasing a version to NPM. Such a step would likely make it impossible to point to a commit on GitHub and have the package still work. But I've verified that it does indeed work when we point to the commit on GitHub, so there's no problem there. [1] zulip#4420 (comment)
In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread [2] for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [4]. We know of one bug that's directly solved by taking this upgrade: issue zulip#4365. I've just tested, and I do indeed start to see avatars on a simulator running iOS 14, where I didn't see them before. [1] See zulip#4420 (comment) for more details on this. [2] zulip#4420 (comment) [3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [4] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file. Fixes: zulip#4245 Fixes: zulip#4365
Following #4296, #4318, #4319, #4320, #4321, #4322, and #4414, complete the RN v0.63 upgrade! 🎉
If, during testing, you encounter an error during the
postinstall
script like either of the errors described in #4420 (comment), try runningrm -rf ios/Pods
and rerunningyarn
; no further action should be necessary. This has worked for me, in testing, when hopping between before and after the upgrade. (As described in that comment, I've already committed the result of running apod update
command in the upgrade commit. That seems to have fixed something that wouldn't be fixed by merely removing ios/Pods.)If, when testing on Android, you get an instant crash, try removing
android/.gradle
and running./gradlew clean
from theandroid
directory, as recommended in a few places in our build-run doc.Fixes: #4245
Fixes: #4365